[hist] Pause filling during snapshot - #23093
Conversation
Test Results 23 files 23 suites 3d 17h 16m 9s ⏱️ For more details on these failures, see this check. Results for commit 313652f. ♻️ This comment has been updated with latest results. |
|
|
||
| std::atomic_flag snapshotter; | ||
| StressInParallel(NThreads, [&] { | ||
| if (!snapshotter.test_and_set()) { |
There was a problem hiding this comment.
If I understand correctly, this means that the first thread to run will start N consecutive snapshots and lead all the other thread to all do Fills.
Should we also test when a second thread is attempting to do a snapshot (to exercise the while loop on line 873)?
The time taken by the snapshot-ing thread between the end of a snapshot and the start of another one is smallish (2 GetBinContent), is there a risk that with this PR the execution now mostly does all the snapshots before (most of) the Fills? (i.e. not quite but almost executing the whole snapshot-ing threads before the other)
There was a problem hiding this comment.
Should we also test when a second thread is attempting to do a snapshot (to exercise the while loop on line 873)?
That's exactly what the "new" test StressSnapshotAtomic does, calling SnapshotAtomic from 4 threads, no? The renamed StressFillSnapshotAtomic then tests the interaction between FillAtomic and SnapshotAtomic, but here we can never have two SnapshotAtomic interacting.
is there a risk that with this PR the execution now mostly does all the snapshots before (most of) the Fills
I don't think so, it also has to destruct the snapshot histogram.
There was a problem hiding this comment.
That's exactly what the "new" test StressSnapshotAtomic does,
Indeed.
The renamed StressFillSnapshotAtomic ... but here we can never have two SnapshotAtomic interacting.
It 'could' (straightforwardly) be extended to have 2 Snapshot threads hence testing the 3 ways interactions.
I don't think so, it also has to destruct the snapshot histogram.
Fair enough.
FillAtomic is much faster than SnapshotAtomic, so in case of heavy contention it would starve the snapshot. By adding a new atomic flag FillAtomic can pause while a Snapshot is running. The successful double collect is still needed if a thread is already in FillAtomic. If SnapshotAtomic is not used, the performance impact of the added check is minimal / not measurable because fSnapshot is always false and already in the cache line of the RHistEngine.
a14b914 to
313652f
Compare
| { | ||
| ROOT::TestSupport::CheckDiagsRAII diagRAII; | ||
| diagRAII.optionalDiag(kWarning, "TKey::TKey", "no public constructor", /*matchFullMessage=*/false); | ||
| diagRAII.optionalDiag(kWarning, "TStreamerInfo::Build", "data member \"fSnapshot\" will not be saved", |
There was a problem hiding this comment.
In which configuration did that appears? (Since fSnapshot is marked transient, we should not be issuing this message)
| std::vector<BinContentType> fBinContents; | ||
|
|
||
| /// Flag to pause filling while a snapshot is ongoing | ||
| mutable std::atomic<bool> fSnapshot{false}; //! |
There was a problem hiding this comment.
Maybe the comment is a bit late, but this name might better convey the fact that this is set while something is ongoing.
| mutable std::atomic<bool> fSnapshot{false}; //! | |
| mutable std::atomic<bool> fSnapshotInProgress{false}; //! |
| std::swap(fAxes, rhs.fAxes); | ||
| std::swap(fBinContents, rhs.fBinContents); |
There was a problem hiding this comment.
Can a snapshot be in progress while this is called? Maybe it's OK to assume that the answer is "no", because moving from something that's in use is likely a problem.
| StressInParallel(NThreads, [&] { | ||
| for (std::size_t i = 0; i < NSnapshotsPerThread; i++) { | ||
| auto snapshot = engine.SnapshotAtomic(); | ||
| // compare_exchange wants a non-const reference... | ||
| int expected = ExpectedBinContent0; | ||
| int actual = snapshot.GetBinContent(0); | ||
| if (actual != expected) { | ||
| binContent0.compare_exchange_strong(expected, actual); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Since the test below also fills while snapshotting, isn't all what this test does (e.g. to ensure that it doesn't deadlock) also included in the one below?
| do { | ||
| while (fSnapshot.load(std::memory_order_relaxed)) { | ||
| // Spin while another snapshot is running | ||
| } | ||
| } while (fSnapshot.exchange(true, std::memory_order_relaxed)); |
There was a problem hiding this comment.
| do { | |
| while (fSnapshot.load(std::memory_order_relaxed)) { | |
| // Spin while another snapshot is running | |
| } | |
| } while (fSnapshot.exchange(true, std::memory_order_relaxed)); | |
| // Spin while another snapshot is running and reserve our turn | |
| bool expected = false; | |
| while (! fSnapshot.compare_exchange_strong(expected, true, std::memory_order_relaxed)); |
3 participants
FillAtomicis much faster thanSnapshotAtomic, so in case of heavy contention it would starve the snapshot. By adding a new atomic flagFillAtomiccan pause while a Snapshot is running. The successful double collect is still needed if a thread is already in FillAtomic.If
SnapshotAtomicis not used, the performance impact of the added check is minimal / not measurable becausefSnapshotis always false and already in the cache line of theRHistEngine.